add PackageBuilder::with_files too allows batch addition of files to a rpm
#194
Conversation
Signed-off-by: Luna <[email protected]>
Signed-off-by: Luna <[email protected]>
|
|
||
| pub fn with_files( | ||
| mut self, | ||
| files: Vec<(impl AsRef<Path>, impl Into<FileOptions>)>, |
There was a problem hiding this comment.
Could we make these explicit generics? It allows the user to specify the types in case of ambiguities.
There was a problem hiding this comment.
Note: I am not a particular fan of tuples, in this particular case, adding a type just for the sake of a public function input might be overkill.
|
I've changed my thinking a bit on the whole API for building RPMs. The trouble is that if you look at building an RPM of even average complexity, the builder approach with pass-by-value just doesn't scale very well. By that I mean mostly that it isn't readable. let built_rpm = PackageBuilder::new(
"rpm-basic",
"2.3.4",
"MPL-2.0",
"noarch",
"A package for exercising basic features of RPM",
)
.epoch(1)
.release("5")
.description("This package attempts to exercise basic features of RPM packages.")
.vendor("Los Pollos Hermanos")
.url("http://www.savewalterwhite.com/")
.vcs("https://github.com/rpm-rs/rpm")
.group("Development/Tools")
.packager("Walter White")
.compression(CompressionType::None)
.build_host("localhost")
.source_date(1681068559)
.provides(Dependency::any("/usr/bin/ls"))
.provides(Dependency::any("aaronpaul"))
.provides(Dependency::any("breaking(bad)"))
.provides(Dependency::config("rpm-basic", "1:2.3.4-5.el9"))
.provides(Dependency::eq("rpm-basic", "1:2.3.4-5.el9"))
.provides(Dependency::eq("shock", "33"))
.requires(Dependency::script_pre("/usr/sbin/ego"))
.requires(Dependency::config("rpm-basic", "1:2.3.4-5.el9"))
.requires(Dependency::greater_eq("methylamine", "1.0.0-1"))
.requires(Dependency::less_eq("morality", "2"))
.requires(Dependency::script_post("regret"))
.conflicts(Dependency::greater("hank", "35"))
.obsoletes(Dependency::less("gusfring", "32.1-0"))
.obsoletes(Dependency::less("tucosalamanca", "444"))
.supplements(Dependency::eq("comedy", "0:11.1-4"))
.suggests(Dependency::any("chilipowder"))
.enhances(Dependency::greater("purity", "9000"))
.recommends(Dependency::any("SaulGoodman(CriminalLawyer)"))
.recommends(Dependency::greater("huel", "9:11.0-0"))
.with_file(
"./tests/assets/SOURCES/example_config.toml",
FileOptions::new("/etc/rpm-basic/example_config.toml").is_config(),
)?
.with_file(
"./tests/assets/SOURCES/multiplication_tables.py",
FileOptions::new("/usr/bin/rpm-basic"),
)?
.with_file(
// I didn't bother filling all of these in yet, obviously it's not complete
"",
FileOptions::new("/usr/lib/rpm-basic").mode(FileMode::Dir { permissions: 0o644 }),
)?
.with_file(
"",
FileOptions::new("/usr/lib/rpm-basic/module").mode(FileMode::Dir { permissions: 0o755 }),
)?
.with_file(
"./tests/assets/SOURCES/module/__init__.py",
FileOptions::new("/usr/lib/rpm-basic/module/__init__.py"),
)?
.with_file(
"./tests/assets/SOURCES/module/hello.py",
FileOptions::new("/usr/lib/rpm-basic/module/hello.py"),
)?
.with_file(
"",
FileOptions::new("/usr/lib/rpm-basic/module").mode(FileMode::Dir { permissions: 0o755 }),
)?
.with_file(
"",
FileOptions::new("/usr/share/doc/rpm-basic").mode(FileMode::Regular { permissions: 0o644 }),
)?
.with_file(
"",
FileOptions::new("/usr/share/doc/rpm-basic/README").is_doc(),
)?
.with_file(
"./tests/assets/SOURCES/example_data.xml",
FileOptions::new("/usr/share/rpm-basic/example_data.xml"),
)?
.with_file(
"",
FileOptions::new("/var/log/rpm-basic/basic.log").is_ghost(),
)?
.with_file(
"",
FileOptions::new("/var/tmp/rpm-basic").mode(FileMode::Dir { permissions: 0o755 }),
)?
.add_changelog_entry(
"Walter White <[email protected]> - 3.3.3-3",
"- I'm not in the meth business. I'm in the empire business.",
1623672000,
)
.add_changelog_entry(
"Gustavo Fring <[email protected]> - 2.2.2-2",
"- Never Make The Same Mistake Twice.",
1619352000,
)
.add_changelog_entry(
"Mike Ehrmantraut <[email protected]> - 1.1.1-1",
"- Just because you shot Jesse James, don't make you Jesse James.",
1619352000,
)
.build()?;Maybe it would be better if we passed by reference instead. This would mean you have to assign ownership to a variable for the builder before proceeding with setting new fields, but it would make it a lot easier to split things up into sections and use loops like: let mut builder = PackageBuilder::new(
"rpm-basic",
"2.3.4",
"MPL-2.0",
"noarch",
"A package for exercising basic features of RPM",
);
builder
.epoch(1)
.release("5")
.description("This package attempts to exercise basic features of RPM packages.")
.vendor("Los Pollos Hermanos")
.url("http://www.savewalterwhite.com/")
.vcs("https://github.com/rpm-rs/rpm")
.group("Development/Tools")
.packager("Walter White");
builder
.provides(Dependency::any("/usr/bin/ls"))
.provides(Dependency::any("aaronpaul"))
.provides(Dependency::any("breaking(bad)"))
.provides(Dependency::config("rpm-basic", "1:2.3.4-5.el9"))
.provides(Dependency::eq("rpm-basic", "1:2.3.4-5.el9"))
.provides(Dependency::eq("shock", "33"));
for req in requirements {
builder.requires(req);
}You could do this previously using assignments (as noted in #193 (comment)), but this way is cleaner and doesn't lean so hard on the compiler to remove a bunch of memcpys. This is just an idea, though. I'm curious what others think. |
|
I quite prefer the api you've proposed to the current one. The current one feels rather clunky and obtuse compared to the proposed one |
|
Your suggested new design is more friendly for new users. I'm glad the suggested code works with my existing coding style of splitting processes like
I consider it is possible to write the chained-style code without the need for a builder variable as like std::process::Command. |
You're probably right, I was just thinking that if |
|
Ok, I've filed #220, we can continue this discussion there. |
This pr address #193 and adds a
with_filesmethod which allows adding multiple files in batches to the rpm. There is a test based ofwith_fileand documentation as well.📜 Checklist
--all-featuresenabled